Skip to content

Conversation

@nachiket-galileo
Copy link
Contributor

No description provided.

@nachiket-galileo nachiket-galileo requested a review from a team as a code owner September 25, 2025 00:42
@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.76%. Comparing base (18aef69) to head (9fe1bee).
⚠️ Report is 90 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #331   +/-   ##
=======================================
  Coverage   83.76%   83.76%           
=======================================
  Files          56       56           
  Lines        3961     3961           
=======================================
  Hits         3318     3318           
  Misses        643      643           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -1,3 +1,5 @@
post_hooks:
# Fix missing status codes in generated API files
- 'python ../../../scripts/patch_status_codes.py "../../../src/galileo/resources/api"'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vamaq Added this new script

Copy link
Contributor

@jrhone jrhone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's check if openapi modified their codegen_templates/endpoint_module.py.jinja file and if we need to pull those changes into our version and re-apply our modifications to that file back on top :)

@nachiket-galileo
Copy link
Contributor Author

Let's check if openapi modified their codegen_templates/endpoint_module.py.jinja file and if we need to pull those changes into our version and re-apply our modifications to that file back on top :)

@jrhone Quick FYI the updates dont work and are still breaking for me


headers["Content-Type"] = "application/json"

headers["X-Galileo-SDK"] = f"galileo-python/{get_package_version()}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you add this manually?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see it's in the jinja template.. interesting that it was missing from this file and not the rest, I guess doesn't matter 🤷🏾‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But actually yeah I haven't checked every file.. github is forcing me to look at each file one at a time 😂

) -> Optional[Union[BulkDeleteDatasetsResponse, HTTPValidationError]]:
if response.status_code == 200:
response_200 = BulkDeleteDatasetsResponse.from_dict(response.json())
return BulkDeleteDatasetsResponse.from_dict(response.json())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it generates with if response.status_code == :
Then the script runs to convert to if response.status_code == 200:
Then the linter makes the change from response_200 = to return?

@jrhone
Copy link
Contributor

jrhone commented Sep 26, 2025

Let's check if openapi modified their codegen_templates/endpoint_module.py.jinja file and if we need to pull those changes into our version and re-apply our modifications to that file back on top :)

@jrhone Quick FYI the updates dont work and are still breaking for me

Gotcha... I'm not sure about using a script to fix what should be built-in functionality.

I mean we're not customizing the functionality.. the tool is just breaking.

I suppose if no other fix then happy to approve, but, what are your thoughts on that?

@jrhone
Copy link
Contributor

jrhone commented Sep 26, 2025

Wasn't possible to separate linter changes and actual changes in different PRs?

If it's a headache don't worry, I pushed the diff through GPT and looks clean.

@jrhone
Copy link
Contributor

jrhone commented Sep 26, 2025

Should we see if an updated version of openapi-python-client fixes it?

Seems we're on 0.25.3

% pip freeze | grep openapi-python-client
openapi-python-client==0.25.3

In version 0.26.0 there was a commit related to http status codes which updated the templates
Support patterned and default HTTP status codes

I thought we had auto upgraded to that version which caused the break but that makes sense now why trying the latest templates didn't fix it for you.. but if we pull in the latest version maybe it will.. maybe not.

Copy link
Contributor

@jrhone jrhone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I guess my only real comment above is what happens if we update our openapi-python-client to the latest.. maybe it will fix the status code issue since there were changes directly related to status codes in the new version.

Ideally we wouldn't fix breaking functionality with a patch.. but otherwise looks good.

@jrhone
Copy link
Contributor

jrhone commented Sep 26, 2025

Oh, and looks like a new version was deployed to API recently that has changes I need for my other tasks.. if you wouldn't mind re-generating with that version since the re-generation breaks for me.

Or feel free to merge this PR in as is but I might bug you to regenerate a fresh one 🙏🏾

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants